RFD-322 -- `/v1/` Disks API #2008

just-be-dev · 2022-12-05T20:47:38Z

This PR is part of #2028

Changes

Old way of fetching disks

GET /organizations/<org_name>/projects/<project_name>/disks/<disk_name>
GET /by-id/disks/<disk_id>

New ways of fetching disks

GET /v1/disks/<disk_id>
GET /v1/disks/<disk_name>?project=<project_id>
GET /v1/disks/<disk_name>?project=<project_name>&organization=<org_id>
GET /v1/disks/<disk_name>?project=<project_name>&organization=<org_name>

Fetching disks attached to an instance

GET /v1/instances/<instance_id>/disks
GET /v1/instances/<instance_name>/disks?project=<project_id>
GET /v1/instances/<instance_name>/disks?project=<project_name>&organization=<org_id>
GET /v1/instances/<instance_name>/disks?project=<project_name>&organization=<org_name>

Attaching and detaching disks to an instance

POST /v1/instances/{instance}/disks/attach  { disk: NameOrId }
POST /v1/instances/{instance}/disks/detach  { disk: NameOrId }

Disk metrics are not included in this PR anymore. There's some work necessary to smooth out pagination which may not be necessary with upcoming metrics changes.

just-be-dev · 2022-12-21T03:08:15Z

nexus/src/app/instance.rs

-        let (.., authz_project, authz_disk, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .organization_name(organization_name)
-                .project_name(project_name)
-                .disk_name(disk_name)
-                .fetch()
-                .await?;
-        let (.., authz_instance, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .project_id(authz_project.id())
-                .instance_name(instance_name)
-                .fetch()
-                .await?;
-
+        let (.., authz_instance) =
+            instance_lookup.lookup_for(authz::Action::Modify).await?;
+        let (.., authz_disk) =
+            disk_lookup.lookup_for(authz::Action::Modify).await?;


The permissions here before were wrong I believe. It was doing a fetch call, which was unnecessary, and not using the right permissions.

just-be-dev · 2022-12-21T03:08:44Z

nexus/src/app/instance.rs

-        let (.., authz_project, authz_disk, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .organization_name(organization_name)
-                .project_name(project_name)
-                .disk_name(disk_name)
-                .fetch()
-                .await?;
-        let (.., authz_instance, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .project_id(authz_project.id())
-                .instance_name(instance_name)
-                .fetch()
-                .await?;
-
+        let (.., authz_instance) =
+            instance_lookup.lookup_for(authz::Action::Modify).await?;
+        let (.., authz_disk) =
+            disk_lookup.lookup_for(authz::Action::Modify).await?;


Same as the attach case. Doing an unnecessary fetch and not using the right permissions.

just-be-dev · 2022-12-21T03:19:11Z

nexus/src/external_api/http_entrypoints.rs

+#[endpoint {
+    method = GET,
+    path = "/v1/disks",
+    tags = ["disks"],
+}]


Warning
I've taken liberties here. This code warrants extra attention.

I've collapsed down the different ways of listing disks into a single endpoint

Previously we had

GET /.../projects/{project}/disks

GET /.../instances/{instance}/disks

Essentially this allows listing all disks in the first case and only disks attached to a particular instance in the second case. I've collapsed this down into a single endpoint.

GET /v1/disks?project={project_id}

GET /v1/disks?instance={instance_id}

If you provide the instance selector then it filters down to disks attached to the instance. Otherwise it gives you all disks in the projects. That makes this endpoint have slightly different requirement semantics on its selectors. In general the selector for the parent resource (project in this case) is required for list endpoints. Because the selector here can be either an instance or project (and their subsequent paths) all the selectors are optional.

Overall I think the usage of this is simplified and it gives one place to go to list disks. Let me know what you think.

In line with #2008 (comment)

I'm not sure what you mean here.

I can see the appeal. I'm not sure I grok the implications but it sounds reasonable!

Ha, well, I did an about face. This no longer applies. The endpoint is now /v1/instances/{instance}/disks which maps to what we had previously.

I guess I'm fine with either. You had me convinced (and enthusiastic!) about a single endpoint with multiple search criteria, but I'm fine with the "about face". What changed your mind?

A few reasons:

Keeping /disks on the end of instances is consistent with the current API so this is the minimal viable change. This is the main one.

A consequence of being able to select on instance or project is that they both had to be optional. That just means there's an extra failure mode here. There's also a bit more to document.

I will say that I like the single endpoint better (what I had) than I like having two endpoints returning the same type of things at different levels of the API (what's there now and before this PR). If we already have a place that we're interacting with disks associated with an instance though, it probably just makes sense to add the listing there.

Overall I would be good to move back to the single disk list endpoint, but we'd need to do something with the attach/detach APIs. I don't think we'd want to have instances/{}/disks/(attach|detach) because the /disks part would be hanging with no valid methods on it. If we dropped /disks like @karencfv original suggestion then /instances/{}/(attach|detach) may be too vague. What are you attaching or detaching? Lastly I just wasn't a fan of chaining names together where we don't have to /disks-attach. I really couldn't think of a better alternative so I just went back to the original.

just-be-dev · 2022-12-21T03:27:27Z

nexus/src/external_api/http_entrypoints.rs

+/// Attach a disk to an instance
+#[endpoint {
+    method = POST,
+    path = "/v1/disks/{disk}/attach",
+    tags = ["disks"],
+}]


Warning
This section warrants extra attention.

With attach and detach actions I took the same sort of approach as I did with listing in that I collapsed it down to the disks endpoint.

Before:

POST /.../instances/{instance}/disks/attach { disk: Name }

POST /.../instances/{instance}/disks/detach { disk: Name}

After

/v1/disks/{disk}/attach { instance: NameOrId }

/v1/disks/{disk}/detach { instance: NameOrId }

Given the overall flatter structure of the API I think this mapping makes slightly more sense. Input welcome.

Unfortunately I don't think this approach will be as clear as the previous one. You generally attach a disk to an instance(vm), and not the other way around. Flipping it around can potentially confuse people and the endpoint itself could be misleading.

What do you think about something like: /v1/instance/{instance}/attach { disk: NameOrId } instead of /v1/disks/{disk}/attach { instance: NameOrId }

We can even go a bit further by having a very explicit endpoint like: /v1/instance/{instance}/attach-disk { disk: NameOrId }

I see what you're saying. The challenge with attach and detach is that it's a two way action. It acts on both the disk and the instance. The benefit of the implementation I have is that there's only a single place to look for disks within the API. If clarification is the main sticking point it could just as well be updated to be /v1/disks/{disk}/attach-to-instance and /v1/disks/{disk}/detach-from-instance. If we go back to hanging it off of instances then I believe we should keep with the previous model of /instance/{instance}/disks/*.

I'm going to roll this back into the original style. I found an issue in my implementation that would require updates at the Nexus app layer to address which I'd rather not do.

Okay, this is reverted to

/v1/instances/{instance}/disks/attach

/v1/instances/{instance}/disks/detach

Agreed with where you landed on this: I'm "talking to" the instance and the object of the attach/detach "verb" is the disk.

karencfv

Nice! Another group of endpoints to tick off the list 🎉

Left a few comments/questions let me know what you think

karencfv · 2022-12-21T03:34:19Z

nexus/src/external_api/http_entrypoints.rs

+#[endpoint {
+    method = GET,
+    path = "/v1/disks/{disk}/metrics/{metric_name}",
+    tags = ["disks"],
+}]


No need to take any action right now, but just as an FYI this endpoint is very likely to change.

karencfv · 2022-12-21T03:42:26Z

nexus/src/app/disk.rs

+                project_selector: Some(project_selector),
+            } => {
+                let disk = self
+                    .project_lookup(opctx, project_selector)?


Just wondering, how much work will be involved to change project_lookup to work without organizations?

Should the implementation change much? If so, how much effort would including organization in the lookup now and removing it almost immediately after take?

It shouldn't be too much work. It's fairly formulaic.

karencfv · 2022-12-21T04:14:03Z

nexus/src/external_api/http_entrypoints.rs

+/// Attach a disk to an instance
+#[endpoint {
+    method = POST,
+    path = "/v1/disks/{disk}/attach",
+    tags = ["disks"],
+}]


Unfortunately I don't think this approach will be as clear as the previous one. You generally attach a disk to an instance(vm), and not the other way around. Flipping it around can potentially confuse people and the endpoint itself could be misleading.

What do you think about something like: /v1/instance/{instance}/attach { disk: NameOrId } instead of /v1/disks/{disk}/attach { instance: NameOrId }

We can even go a bit further by having a very explicit endpoint like: /v1/instance/{instance}/attach-disk { disk: NameOrId }

nexus/src/external_api/http_entrypoints.rs

davepacheco · 2022-12-21T18:59:38Z

nexus/src/app/disk.rs

        }
    }

    pub async fn project_delete_disk(


nit and kind of a general comment: these used to be called project_$action_$subresource because they were nested under the Project, but that's no longer really true. It seems like all of these could now just be called $resource_$action (i.e., disk_delete() here). That probably applies to a bunch of other functions too.

I've added a note in the parent issue to tackle this as a follow up.

davepacheco · 2022-12-21T19:00:50Z

nexus/src/app/instance.rs

-                .fetch()
-                .await?;
-
+        let (.., authz_instance) =


What happens if the disk is in a different Project? (Also, do we have a test for that?)

Before, it wasn't possible to specify this. Now it seems like it is.

I've updated the code here. It now uses a lookup for the instance and gets the authz_project then does a lookup for the disk with the project id as the root of the lookup. I think what you're saying still may apply though. If you try to attach a disk by UUID then the project id in the lookup would be ignored and the disk UUID would be used instead. I'll add a check for this, good catch.

Fix is added in 8f79dc6. I only added it to the attach case because the detach case only operates on whats already attached so it's an unnecessary check there. In the error message I noticed that they must be in the same project. This technically tells the user something about the relationship of the resources they may not have known, but I think that's fine because the step before should ensure they have modify permissions on both resources.

Great. I'm not sure what the plan is for TODO-v1 but this seems like a test we want sooner rather than later because it feels easy to break without noticing and it seems really bad if we accidentally ship it that way.

All the TODO-v1 comments will be addressed before switching to the v1 endpoints.

just-be-dev · 2022-12-21T19:14:52Z

nexus/src/external_api/http_entrypoints.rs

+    rqctx: Arc<RequestContext<Arc<ServerContext>>>,
+    path_params: Path<params::InstancePath>,
+    query_params: Query<params::OptionalProjectSelector>,
+    disk_to_attach: TypedBody<params::DiskPath>,


I want to acknowledge that using DiskPath here is weird. My plan is to eventually convert all the *Path structs into *Identifier for consistency. Whether they're used in the path or provided via the body doesn't really matter given it's just different places to specify a resource.

just-be-dev · 2022-12-21T19:15:23Z

nexus/tests/output/uncovered-authz-endpoints.txt

@@ -1,7 +1,9 @@
 API endpoints with no coverage in authz tests:


I know this is a mess. I will address it.

…nstance

ahl

looks good; I think there's a problem with the disk list and disk list metrics endpoints

nexus/types/src/external_api/params.rs

ahl · 2022-12-21T21:47:00Z

nexus/types/src/external_api/params.rs

+    #[serde(flatten)]
+    pub project_selector: ProjectSelector,
+    #[serde(flatten)]
+    pub pagination: PaginatedByName,


as below: I'm not sure how this works.

Fixed in 7f608d1

ahl · 2022-12-21T21:49:47Z

nexus/src/external_api/http_entrypoints.rs

+    let handler = async {
+        let opctx = OpContext::for_external_api(&rqctx).await?;
+        let project_lookup =
+            nexus.project_lookup(&opctx, &query.project_selector)?;


this is invalid when query.pagination is.page matches the WhichPage::Next variant.

Fixed in Fixed in 7f608d1. I also updated the integration tests to verify

ahl · 2022-12-21T21:57:26Z

nexus/types/src/external_api/params.rs

+#[derive(Deserialize, JsonSchema)]
+pub struct DiskSelector {
+    #[serde(flatten)]
+    pub project_selector: Option<ProjectSelector>,


do we prefer this style to

Suggested change

pub project_selector: Option<ProjectSelector>,

pub project: Option<NameOrId>,

pub organization: Option<NameOrId>,

Does it allow for additional code sharing for these nested, flattened selector structures?

By encoding a selector at every level (which itself is a resource + possible parent selector) we can flatten both the creation of selectors and the lookup functions down to only concerning itself with two cases. If you look at the endpoint handlers they're always using using a selector and potentially a path param. My thought here is that this general shape will be easier to encode in a macro. Once organization goes away and project is flattened a bit of the structure can be reduced though.

Co-authored-by: Adam Leventhal <ahl@oxide.computer>

This reverts commit 2f75756.

just-be-dev · 2023-01-18T16:20:28Z

I'm going to go ahead and merge this for the sake of forward progress. @ahl if you notice anything else let me know and I'll follow up on it.

See #2028 This PR adds V1 versions of the developer facing networking APIs. Firewall rules aren't in this PR. I'll do that in a follow up. ## New Endpoints All endpoints here are scoped via query parameters by their parent selectors. So for `vpc-subnets` if you provide the vpc by name, you'll also need to provide the project. If the projected is specified by name you'll need the org. So `/v1/vpc-subnet?vpc={vpc_id}`, `/v1/vpc-subnet?vpc={vpc_name}&project={project_id}`, etc ```shell # Scoped via ?project={project} GET /v1/vpcs # List VPCs POST /v1/vpcs # Create a VPC GET /v1/vpcs/{vpc} # Get a VPC DELETE /v1/vpcs/{vpc} # Delete a VPC # Scoped via ?vpc={vpc} GET /v1/vpc-subnets # List VPC subnets POST /v1/vpc-subnets # Create a VPC subnet GET /v1/vpc-subnets/{subnet} # Get a VPC subnet DELETE /v1/vpc-subnets/{subnet} # Delete a VPC subnet # Scoped via ?vpc={vpc} GET /v1/vpc-routers # List VPCs POST /v1/vpc-routers # Create a VPC GET /v1/vpc-routers/{router} # Get a VPC DELETE /v1/vpc-routers/{router} # Delete a VPC # Scoped via ?router={router} GET /v1/vpc-router-routes # List VPC router routes POST /v1/vpc-router-routes # Create a VPC router route GET /v1/vpc-router-routes/{route} # Get a VPC router route DELETE /v1/vpc-router-routes/{route} # Delete a VPC router route # Scoped via ?instance={instance} GET /v1/network-interfaces # List network interfaces POST /v1/network-interfaces # Create network interface GET /v1/network-interfaces/{interface} # Get a network interface DELETE /v1/network-interfaces/{interface} # Delete network interface ``` For a quick overview of the entire API (and the changes introduced in this PR) check out the [nexus_tags.txt file](https://github.com/oxidecomputer/omicron/pull/2057/files#diff-482331ae638fae81e451868307fcd2f8836600a2ac552c965da987a402d94e54) ## Notable changes - I pulled app level network interface functions out into their [own file](https://github.com/oxidecomputer/omicron/pull/2057/files#diff-4d675c5f8ef51a7a12255d0a6cd593b4bfb7b77dbf5e791c97abf9e874a588bf). - I've slowly started to update the naming convention of app and datastore level functions to be less nested (and closer to the operation names) as per [this comment](#2008 (comment)). It's a little bit inconsistent now but the intent is to do a larger follow up to clean these all up. At present I'm just updating them as I come across them. - I've begun moving all possible contents of an http entrypoint into its handler for more timing information as per [this comment](#2206 (comment))

just-be-dev and others added 30 commits November 17, 2022 15:33

Test impl of new endpoint format

233dc6d

ResourceIdentifier -> NameOrId

0be1778

Add docs, remove unneeded chunk

6e3210f

Use v1 prefix, convert instance delete

a2681b9

Push up diplay error

18bc9cf

ReferenceIdentifer -> NameOrId in nexus.json

07ec450

Update nexus.json to fix build issues

5b2f217

Change v1 to a prefix instead of postfix

8631e9f

Normalize id lookups and path selection

41672aa

Add v1 create instance api

954ffdb

Add instance list endpoint

c8d191b

Add instance migrate endpoint

6708bac

Add instance reboot endpoint

26b5651

Add instance start endpoint

f70f84f

Add instance stop endpoint

e6374fe

Add serial console endpoints

cdaa977

Workaround for multiple emits

d0a19c6

Implement NameOrId for display to try resolve dropshot error

f439f59

Flip the order of id/name in the NameOrId enum

b4e818a

Add untagged to NameOrId

7bdfeb0

Update NameOrId to be labeled

cf5f540

RFD-322 PoC Revision 2 (#1995)

8c7ce21

Move version to postfix

87f4d25

Add v1 disk list endpoint

1596ed2

Merge main int rfd-322-poc

393283d

Merge branch 'rfd-322-poc' into rfd-322-poc-disks

ae73d95

Add new disk create endpoint

3d69694

Add disk view

57549c7

Always do a lookup, even when only handling UUID

2a87558

Merge branch 'main' into rfd-322-poc

0b227b0

just-be-dev added 2 commits December 20, 2022 16:40

Fix final failing tests

e02cd60

Merge branch 'main' into rfd-322-poc-disks

ded1119

just-be-dev commented Dec 21, 2022

View reviewed changes

karencfv reviewed Dec 21, 2022

View reviewed changes

just-be-dev commented Dec 21, 2022

View reviewed changes

nexus/src/external_api/http_entrypoints.rs Outdated Show resolved Hide resolved

davepacheco reviewed Dec 21, 2022

View reviewed changes

Move back to old pattern of working with disks attached to instances

4e602d2

just-be-dev commented Dec 21, 2022

View reviewed changes

Add checks to ensure disk from different project can be attached to i…

8f79dc6

…nstance

ahl reviewed Dec 21, 2022

View reviewed changes

Update nexus/types/src/external_api/params.rs

2f75756

Co-authored-by: Adam Leventhal <ahl@oxide.computer>

just-be-dev marked this pull request as draft December 21, 2022 23:18

just-be-dev added 3 commits January 3, 2023 15:37

Merge branch 'main' into rfd-322-poc-disks

5c2366d

Revert "Update nexus/types/src/external_api/params.rs"

8ca6b35

This reverts commit 2f75756.

Merge branch 'main' into rfd-322-poc-disks

898002d

just-be-dev mentioned this pull request Jan 6, 2023

Update pagination to support v1 API patterns #2111

Merged

just-be-dev added 2 commits January 11, 2023 16:32

Merge branch 'main' into rfd-322-poc-disks

072c389

Drop metrics for v1; update disk integration tests; update pagination

7f608d1

just-be-dev marked this pull request as ready for review January 13, 2023 04:18

just-be-dev added 3 commits January 12, 2023 23:24

Fix unauth test

2b9369a

Restore disk metrics coverage

f357742

Revert pub change

178e86c

just-be-dev merged commit 7dc9b35 into main Jan 18, 2023

just-be-dev deleted the rfd-322-poc-disks branch January 18, 2023 16:20

just-be-dev mentioned this pull request Feb 9, 2023

RFD-322 Networking #2057

Merged

just-be-dev mentioned this pull request Mar 3, 2023

[nexus] List allocated IPs in a VPC subnet #2476

Open

		@@ -1,7 +1,9 @@
		API endpoints with no coverage in authz tests:

	pub project_selector: Option<ProjectSelector>,
	pub project: Option<NameOrId>,
	pub organization: Option<NameOrId>,

RFD-322 -- /v1/ Disks API #2008

RFD-322 -- /v1/ Disks API #2008

Uh oh!

Conversation

just-be-dev commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RFD-322 -- `/v1/` Disks API #2008

RFD-322 -- `/v1/` Disks API #2008

just-be-dev commented Dec 5, 2022 •

edited

Loading

just-be-dev Dec 21, 2022 •

edited

Loading

just-be-dev Dec 21, 2022 •

edited

Loading

just-be-dev Dec 21, 2022 •

edited

Loading

just-be-dev Dec 21, 2022 •

edited

Loading